Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: reduce the number of decimals shown for simulation detail amounts #24036

Merged
merged 8 commits into from
Apr 23, 2024
Merged

feat: reduce the number of decimals shown for simulation detail amounts #24036

merged 8 commits into from
Apr 23, 2024
+249 −191

Conversation

dbrans
Copy link
Contributor

@dbrans dbrans commented Apr 15, 2024 •
edited
Loading

Description

This PR does two things:

  • Converts balance change amounts to BigNumber. The previous Amount definition was redundant.
  • Formats token amounts as follows:

|amount| < 1: We display a maximum of 3 significant digits.

|amount| >= 1: We display all digits left of the decimal point. We also display some decimal places for smaller amounts, and gradually reduce the decimal precision as the amount gets bigger until we only show whole numbers.

Users are still able to see the original amount, subject to the maximum precision supported by Intl.NumberFormat.

Here is how different amounts are rendered:

Screenshot 2024-04-08 at 14 14 36

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2343

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@github-actions GitHub Actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@dbrans dbrans added the team-confirmations-system DEPRECATED: please use "team-confirmations" label instead label Apr 15, 2024
@dbrans dbrans marked this pull request as ready for review April 16, 2024 12:47
@dbrans dbrans requested review from a team as code owners April 16, 2024 12:47
@sleepytanya
Copy link
Contributor

Works great!

Screenshot 2024-04-16 at 16 54 51 Screenshot 2024-04-16 at 16 56 37 Screenshot 2024-04-16 at 16 58 11

sleepytanya
sleepytanya previously approved these changes Apr 16, 2024
View reviewed changes
@dbrans dbrans requested review from a team as code owners April 16, 2024 23:09
@dbrans dbrans changed the base branch from develop to dbrans/locale-fix April 16, 2024 23:14
@dbrans dbrans changed the base branch from dbrans/locale-fix to develop April 17, 2024 00:06
@metamaskbot
Copy link
Collaborator

Builds ready [93843bd]
Page Load Metrics (1489 ± 761 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint583881397335
domContentLoaded9119372914
load46397214891585761
domInteractive9119372914
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 1.26 KiB (0.02%)
  • common: 0 Bytes (0.00%)

@codecov Codecov
Copy link

codecov bot commented Apr 17, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.48%. Comparing base (358f0bb) to head (85924bc).
Report is 14 commits behind head on develop.

❗ Current head 85924bc differs from pull request most recent head 1086855. Consider uploading reports for the commit 1086855 to get more accurate results

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #24036      +/-   ##
===========================================
+ Coverage    67.47%   67.48%   +0.01%     
===========================================
  Files         1257     1259       +2     
  Lines        49227    49239      +12     
  Branches     12822    12826       +4     
===========================================
+ Hits         33215    33227      +12     
  Misses       16012    16012

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@metamaskbot
Copy link
Collaborator

Builds ready [ee0d144]
Page Load Metrics (1098 ± 646 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint7352516210752
domContentLoaded12150423316
load60318810981345646
domInteractive12150423316
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 1.26 KiB (0.02%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [1086855]
Page Load Metrics (1586 ± 738 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint793791628239
domContentLoaded1289352311
load56385015861537738
domInteractive1289352311
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 1.26 KiB (0.02%)
  • common: 0 Bytes (0.00%)

matthewwalsh0
matthewwalsh0 previously approved these changes Apr 23, 2024
View reviewed changes
@metamaskbot
Copy link
Collaborator

Builds ready [85924bc]
Page Load Metrics (969 ± 592 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint621741003015
domContentLoaded105119105
load5029769691232592
domInteractive95119105
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 1.36 KiB (0.02%)
  • common: 0 Bytes (0.00%)

@sleepytanya
Copy link
Contributor

Just re-testing - works beautifully:
Screenshot 2024-04-23 at 11 24 09
Screenshot 2024-04-23 at 11 24 02

@dbrans dbrans merged commit c33c5df into develop Apr 23, 2024
76 of 79 checks passed
@dbrans dbrans deleted the dbrans/amounts branch April 23, 2024 17:24
@github-actions github-actions bot locked and limited conversation to collaborators Apr 23, 2024
@metamaskbot metamaskbot added the release-11.16.0 Issue or pull request that will be included in release 11.16.0 label Apr 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sleepytanya sleepytanya sleepytanya approved these changes

@matthewwalsh0 matthewwalsh0 matthewwalsh0 approved these changes

Assignees
No one assigned
Labels
QA Passed release-11.16.0 Issue or pull request that will be included in release 11.16.0 team-confirmations-system DEPRECATED: please use "team-confirmations" label instead
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dbrans @sleepytanya @metamaskbot @matthewwalsh0